[이유나] sprint1 #35
[이유나] sprint1 #35
Conversation
There was a problem hiding this comment.
유나님 첫번째 PR 제출 고생하셨습니다!
리뷰 드린 코멘트는 모두 필수 반영사항은 아니고 코드에 정답은 없으니
보고 마음에 드시는 방향으로 수정하시면 좋겠습니다!
벌써 2주차 작업 들어가셨다니 대단해요! 2주차도 화이팅입니다~
- git commit 단위를 나누시는 연습을 해보시면 좋을 것 같아요. index page css 추가, html 수정과 같은 식으로 작게 쪼개시면 시점을 돌리시기도 더 좋답니다.
- 불필요한 이미지들이 있는 것 같아요! 시간이 지나면 구분이 어려우니 지금 안 쓰시는 이미지들은 지우시는 것 추천드려요!
- 이름을 짓는 것은 생각보다 어렵고 연습이 필요한 부분이에요~ 이름 지으실때 고민해보시면 코드의 가독성 및 유지보수 향상에 도움이 될거에요!
There was a problem hiding this comment.
❗️ 수정요청
파일명을 Group 19.png처럼 작성하면 어떤 이미지인지 바로 알기 어렵고, 유지보수 시 파악하기 어렵습니다.
가능하면 이미지의 용도를 알 수 있는 의미 있는 이름으로 변경하는 것이 좋습니다.
해당 이미지의 경우 logo.png로 작성하면 파일명을 보자마자 로고라는 것을 알 수 있습니다.
또한, 띄어쓰기 대신 하이픈(-)이나 언더스코어(_)를 사용하면 코드 작성 시 불필요한 오류를 방지할 수 있어요!
그러니 자세한 이름, 공백 미사용, 그리고 동일한 네이밍 룰을 이용해 이미지명을 지으시는 것을 추천드려요!
There was a problem hiding this comment.
❗️ 수정요청
해당 이미지 파일의 크기를 보면 588× 444로 되어 있습니다.
저희는 반응형 사이트를 만들고 있고, 이런 이미지들은 모바일에서도 사용할 것이기 때문에 추출하실때, 1배수가 아닌 2배수로 추출해주시는 것이 화질면에서 좋습니다.
아래처럼 figma export 탭에서 2x 로 설정하고 추출해주세요~
There was a problem hiding this comment.
❗️ 수정요청
로고이미지가 여러개가 존재하네요~ 필요한 이미지만 남기시는 것을 추천드려요.
There was a problem hiding this comment.
❗️ 수정요청
귀여운 파일명이네요~ 하지만 어떤 페이지인지 알 수 있는 이름으로 변경하시는 것을 추천드려요. 아마 sign-up 이시겠죠?
There was a problem hiding this comment.
💬 여담
github에서 코드리뷰를 할때는 파일 변경사항을 보고 리뷰를 드립니다~
가능하면 주차에 해당하는 파일만 포함시켜주셔야 제가 리뷰가 가능해요!
변경사항이 없는 부분은 files changed 탭에서 노출되지 않아서 코멘트 다는 것이 불가능합니다.
이번에 올려주신 부분은 다음에 많이 변경될 것 같아서 괜찮을 것 같습니다만 다음부터는 주차에 해당하는 변경사항만 올려주세요~
| <header> | ||
| <div class="container"> | ||
| <div class="icon"></div> | ||
| <a href="../login/login.html" class="login"> 로그인</a> |
There was a problem hiding this comment.
❗️ 수정요청
공백은 css 상 의도하지 않은 결과를 만들 수 있습니다~ 불필요한 공백은 제거해주세요~
| <a href="../login/login.html" class="login"> 로그인</a> | |
| <a href="../login/login.html" class="login">로그인</a> |
| <div class="container"> | ||
| <div class="navDesc"> | ||
| <h4>일상의 모든 물건을 <br /> 거래해 보세요</h4> | ||
| <button><a href="/items.html">구경하러 가기</a></button> |
There was a problem hiding this comment.
❗️ 수정요청
로그인 버튼을 눌렀을 때 './items.html'로 이동시켜주는 역할을 하니, 앵커 태그만 남겨두셔도 될 것 같습니다~
버튼 요소처럼 보여도 꼭 버튼 태그로 작성하실 필요는 없습니다!
해당 태그의 역할에 따라 태그를 작성하시는 것이 더 시맨틱하게 작성하는 방법입니다.
| .navDesc { | ||
| display: flex; /* 중앙 정렬을 위해 flexbox 사용 */ | ||
| justify-content: center; /* 가로 중앙 정렬 */ | ||
| align-items: center; /* 세로 중앙 정렬 */ | ||
| flex-direction: column; /* 텍스트와 버튼을 세로로 배치 */ |
There was a problem hiding this comment.
💊 제안
주석을 다는 것은 좋은 습관입니다. 다만 주석또한 읽어야하는 정보라 중복되지 않는 내용을 입력해주시는 것이 가독성면에서 더 좋아요.
지금 주석은 css 를 통해 파악할 수 있는 내용이라 없어도 될 것 같습니다~
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| overflow-x: hidden; | ||
| } | ||
| a {color: inherit; text-decoration: none; outline: none} a:hover, a:active {text-decoration: none;} |
There was a problem hiding this comment.
💊 제안
간략한 reset css 속성들이라 따로 분리하지 않으시고 한 파일에 작성하셨나봐요~
하지만 필요시 특정 태그의 스타일을 초기화하는건 번거로운 일이고, 이를 염두에 두고 css 작성을 해야하니 가능하면 처음부터 브라우저 스타일을 초기화하시고 reset.css로 분리하시고 작업하시는 것을 추천드립니다!
구글에 reset.css, normalize.css로 검색해보시면 미리 작성된 css 파일이 있으니 확인해보시고 추가해보세요~
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| overflow-x: hidden; | ||
| } | ||
| a {color: inherit; text-decoration: none; outline: none} a:hover, a:active {text-decoration: none;} |
There was a problem hiding this comment.
❗️ 수정요청
한 파일내에서는 같은 룰을 공유하는 것이 좋습니다. 또한 적절한 개행은 가독성에 도움이됩니다. 이 부분도 다른 부분처럼 개행하시는 것을 추천드려요~
| * { | |
| margin: 0; | |
| padding: 0; | |
| box-sizing: border-box; | |
| overflow-x: hidden; | |
| } | |
| a {color: inherit; text-decoration: none; outline: none} a:hover, a:active {text-decoration: none;} | |
| * { | |
| margin: 0; | |
| padding: 0; | |
| box-sizing: border-box; | |
| overflow-x: hidden; | |
| } | |
| a { | |
| color: inherit; text-decoration: none; outline: none | |
| } | |
| a:hover, a:active { | |
| text-decoration: none; | |
| } |
2 participants
요구사항
기본
https://ubiquitous-cassata-6e20ac.netlify.app/
네트리파이 주소입니다.
심화
주요 변경사항
스크린샷
멘토에게